Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always set debugger to true in kernelspec #1191

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

ianthomas23
Copy link
Collaborator

Currently the boolean debugger flag in the kernelspec file is determined at build time depending upon whether debugpy is installed or not. I think it should always be True, on the basis that ipykernel supports a debugger in principle and the details of the available debugging are determined at runtime.

This change has no effect on PyPI wheels or conda packages as the flag is always True in those, it only affects building from source. Because debugpy is a compulsory dependency in the pyproject.toml the flag is always True except when doing an editable install with build isolation (pip install -e .) in which case it is False. This is the recommended approach for installing from source specified in the README.md, and is how I came across this problem.

I have also updated test_debugger.py. There is a CI run that explicitly doesn't have debugpy installed and runs test_debugger.py, but unfortunately in this situation all the tests in that file are skipped. So this CI run only really tests that there is no debugger-related functionality in any of the other test files. I have changed test_debugger.py so that all tests are run regardless of whether debugpy is installed or not, and the asserts are different in each situation. Often without debugpy installed the tests don't do anything particularly interesting, but they do confirm that ipykernel doesn't block, or segfault, or raise an error in the non-debugpy situation.

The test_debugger.py tests are a little less readable now because of the new if statements. An alternative approach would be to keep test_debugger.py as it is and add a new test_not_debugger.py file.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@blink1073 blink1073 merged commit 4868558 into ipython:main Jan 13, 2024
30 of 33 checks passed
@@ -66,7 +66,7 @@ def get_kernel_dict(extra_arguments: list[str] | None = None) -> dict[str, Any]:
"argv": make_ipkernel_cmd(extra_arguments=extra_arguments),
"display_name": "Python %i (ipykernel)" % sys.version_info[0],
"language": "python",
"metadata": {"debugger": _is_debugpy_available},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _is_debugpy_available import/definition should have been removed as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I will include cleanup in a PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks.

@ianthomas23 ianthomas23 deleted the kernelspec_debugger_true branch February 14, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants